Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: update gating for chat component #1550

Merged
merged 3 commits into from
Dec 9, 2024
Merged

Conversation

alangsto
Copy link
Contributor

@alangsto alangsto commented Dec 9, 2024

COSMO-574

Enrollment based gating for the chat feature should be toggle-able by a frontend setting. This PR introduces the ENABLE_XPERT_AUDIT setting which is used to determine whether or not audit learners should have access to the chat tool.

Additionally, a new prop should be passed to the Xpert component, which can be used to determine whether or not a given learner is eligible to upgrade in a specific course.

This PR should not be merged until after edx/frontend-lib-learning-assistant#71 has been merged and a new version of the library has been released.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.81%. Comparing base (b09bcbd) to head (bfa4b4e).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
src/index.jsx 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1550   +/-   ##
=======================================
  Coverage   89.81%   89.81%           
=======================================
  Files         325      325           
  Lines        5587     5598   +11     
  Branches     1384     1357   -27     
=======================================
+ Hits         5018     5028   +10     
- Misses        553      555    +2     
+ Partials       16       15    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

courseId={courseId}
contentToolsEnabled={contentToolsEnabled}
unitId={unitId}
isUpgradEligible={isUpgradeEligible}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: small typo, guessing you meant to name this isUpgradeEligible?

@@ -42,19 +47,30 @@ const Chat = ({

const shouldDisplayChat = (
enabled
&& (hasVerifiedEnrollment || isStaff) // display only to verified learners or staff
&& (hasValidEnrollment || isStaff)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this rename, makes more sense since what counts as "verified" is kinda blurred by the enrollments we chose to use

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1!

@@ -175,6 +175,7 @@ initialize({
CHAT_RESPONSE_URL: process.env.CHAT_RESPONSE_URL || null,
PRIVACY_POLICY_URL: process.env.PRIVACY_POLICY_URL || null,
SHOW_UNGRADED_ASSIGNMENT_PROGRESS: process.env.SHOW_UNGRADED_ASSIGNMENT_PROGRESS || false,
ENABLE_XPERT_AUDIT: process.env.ENABLE_XPERT_AUDIT || false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codecov seems to be unhappy with these lines not being covered, not sure we need (or are planning on) testing on these though?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These aren't normally tested as part of unit tests!

Copy link
Member

@varshamenon4 varshamenon4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@@ -48,6 +48,13 @@ export const VERIFIED_MODES = [
'paid-bootcamp',
] as const satisfies readonly string[];

export const AUDIT_MODES = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

&& (
VERIFIED_MODES.includes(enrollmentMode)
// audit learners are only eligible if Xpert has been explicitly enabled for audit
|| (AUDIT_MODES.includes(enrollmentMode) && getConfig().ENABLE_XPERT_AUDIT)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@@ -42,19 +47,30 @@ const Chat = ({

const shouldDisplayChat = (
enabled
&& (hasVerifiedEnrollment || isStaff) // display only to verified learners or staff
&& (hasValidEnrollment || isStaff)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1!

Copy link
Contributor

@schenedx schenedx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@rijuma rijuma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alangsto alangsto merged commit dafdcad into master Dec 9, 2024
7 checks passed
@alangsto alangsto deleted the alangsto/update_chat_gating branch December 9, 2024 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants